-
Notifications
You must be signed in to change notification settings - Fork 13.5k
vulkan: change an assertion and minify others #10328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src1 can go down the first pipeline as nullptr and src0 only needs to be checked once this means the assertion is only required to check if the type is GGML_TYPE_F16 and can usually be skipped
…ma.cpp into vulkan-assertion
|
"IDE told me to" isn't by itself enough to warrant a change, in my opinion. Especially Visual Studio and MSVC seem to be very quick to warn about potential issues. If you have a good reason (performance, safety, readability, etc) to refactor code, that's different. But I don't really see that here. Combining makes the assertions harder to read and at least one of your changes isn't even correct. I'd have to put in some work to check all of them, and I don't see the benefit. |
|
@FirstTimeEZ enough, take a break. |
|
You caught the problem I saw, so the assertion changes might be correct now. However, combining assertions is a style decision, not a performance decision, and I prefer to keep them as is. None of the changes you made have "obvious performance reasons". Improving the logic for SOFT_MAX pipeline selection to remove the assertion there is different, I would accept that change on its own. So if you clean up this PR to only do that, as it was when you opened it, I'll check it and merge it once it's ready. But let me also say that insulting me is not okay. Deleting or editing your comments doesn't mean I don't see what you wrote. I don't have a personal issue with you, I'm not clueless and my comments on your PR aren't stupid. I wrote most of the Vulkan backend and it's on me to make sure it keeps working. I should have put the issue I found into my comment, but then again you could have also just asked and given me a little time to respond. |
Uh oh!
There was an error while loading. Please reload this page.